Skip to content
This repository has been archived by the owner on Jun 3, 2020. It is now read-only.

tendermint-rs: Disallow a block height of 0 #234

Merged
merged 1 commit into from
Apr 20, 2019
Merged

tendermint-rs: Disallow a block height of 0 #234

merged 1 commit into from
Apr 20, 2019

Conversation

tarcieri
Copy link
Contributor

Tendermint itself says: "Height must be greater than 0" (code -32603)

@tony-iqlusion
tendermint-rs: Disallow a block height of 0
ed5f9d0 
Tendermint itself says: "Height must be greater than 0" (code -32603)
@tarcieri tarcieri force-pushed the tendermint-rs/block-height-parsing branch from 954d0c4 to ed5f9d0 Compare April 19, 2019 18:01
@tarcieri tarcieri requested review from liamsi and zmanian April 19, 2019 18:23
liamsi
liamsi reviewed Apr 19, 2019
View reviewed changes
Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While (in Tendermint) the block height indeed needs to be greater than 0 it seems like the height fields (in proposal etc) are only checked against >= 0:
https://github.com/tendermint/tendermint/blob/4253e67c07c69be6d7f7263ab03944ce30a9fc90/types/proposal.go#L51

I would want to double check if this isn’t intentional in Tendermint before mergin this.

@tarcieri
Copy link
Contributor Author

@liamsi sounds good, will hold off.

I know there are a few cases where the block height is represented as 0 (e.g. in priv_validator_state.json) as a placeholder for an uninitialized state.

My personal preference in Rust would be to disallow 0 as a valid block height and use Option<block::Height> to represent potentially uninitialized cases.

@tarcieri tarcieri closed this Apr 19, 2019
@tarcieri tarcieri reopened this Apr 19, 2019
@liamsi
Copy link
Contributor

liamsi commented Apr 20, 2019

My personal preference in Rust would be to disallow 0 as a valid block height and use Optionblock::Height to represent potentially uninitialized cases.

That sounds good to me 👍 Unfortunately, golang doesn't provide us with the same expressiveness as Rust.

@tarcieri tarcieri merged commit 9353fa2 into master Apr 20, 2019
@tarcieri tarcieri deleted the tendermint-rs/block-height-parsing branch April 20, 2019 12:58
This was referenced Apr 21, 2019
Merged
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@liamsi liamsi liamsi left review comments

@zmanian zmanian Awaiting requested review from zmanian

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tarcieri @liamsi @tony-iqlusion